-
Notifications
You must be signed in to change notification settings - Fork 369
Merging Private Branch contents from Kevin's Repo. #888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
like 'ESAPI.enableLegCannonModeAndGetMyAssFired.justification' better. ;-)
…led if someone wants to use it.
Updating parameter null check to test null case. Removing null check on property result (if null ConfigurationException is thrown). Simplifying return from method to verify response is not empty.
Adding branch testing for ESAPI.isMethodExplicitlyEnabled behavior to account for parameter cases. Only case not covered is providing an ESAPI.properties that does not contain the new key.
using the SecurityConfigurationWrapper to verify remaining test case when a ConfigurationException is thrown when the new property is missing or undefined.
Initial code to disable encodeForSQL
… is, despite best intentions.
Plus minor type fix ('class' ==> 'method').
…idn't affect HTTPUtilities.getFileUploads interaces anyway.
…to include its summary description.)
kwwall
approved these changes
Jun 27, 2025
Contributor
kwwall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be biased, but LGTM
Collaborator
Author
|
I don't know how to get any more clear than the writeup in the PoC.
Initially I had started to do some work to pull down a local oracle database and get it running from Docker but then it was correctly pointed out that there are hundreds of JDBC drivers available since 2007 and pinpointing exactly where the behavior manifests and when was genuinely beyond any rational scope, so our reaction is primarily based on the report itself, which clearly demonstrates failure. As someone who has worked as a red teamer in the past, sometimes you DO NOT need to know exactly how or why an exploit works, as long as it works.
In short, we know that with some JDBC drivers, you will be protected, which appears to be your case. However, as the PoC exploit demonstrates, there are clearly some JDBC drivers where this isn't the case, and (Kevin/Jeremiah can freely correct my memory here) we made the decision to deprecate the Oracle Codec because we are unable to guarantee a safe harbor. Part of the reasoning is that the Database Codecs we inherited, have practically zero unit tests, and because of the myriad of configurations involved with not just properly writing unit tests, but downloading and running them against a live database is so far beyond the time scope, AND then picking "the right" JDBC drivers, it's too much. TO do it correctly we'd have to test against every JDBC driver in existence. We could sample, but that would just create a cat/mouse game with future exploiters. The algorithm would be, "find the one JDBC version where this doesn't work, then get a CVE# from Mitre against ESAPI. This is as opposed to our Codecs for HTML/PercentCodecs for example, that have a universal standard declared: we omit semicolons from our entity detection because modern browsers will render & even though by the spec, the correct behavior should only be & So we do sort of cater to browser vendors in THAT way for the codecs most people normally use. Contrast: 3-6 popular browsers to test, vs 95 Oracle JDBC drivers.
But this is universal for HTML/PercentCodecs, and potentially unique for every JDBC driver implementation. Oracle in particular, can make changes whenever they want. So given the fact that we can't test to where we would feel safe, the decision was to deprecate the class, and if future people WANT or NEED the behavior of that Codec, they can snag it from a prior ESAPI commit and compile their own version. The last part of the conversation back last July was this: People shouldn't be relying on that OracleCodec in the first place. To be frank: it isn't doing anything special in the first place, the algorithm is "see a ', output ''"
You don't need ESAPI for that.
The original intent of the library before Kevin and I inherited it, was to be an emergency drop-in for a java web application that just got hacked, the idea was to use it to safeguard the app until things like PreparedStatements could be written. The encoder functions turned out to have legs as it's still pretty uncommon for web frameworks to offer things like HTML escaping out of the box. I'll put words in Kevin's mouth here, but I think we both agree that the chances that there are dozens if not hundreds of applications in the wild that implemented the OracleCodec that never got around to rewriting their queries into PreparedStatements, and at least on MY part, I'm absolutely fine with forcing the ecosystem to do an update.
…-- Matt Seil
Manager/Princ Application Security
Member ACM/OWASP
Sent with Spark
On Jan 9, 2026, 6:59 AM -0700, f-delahaye ***@***.***>, wrote:
f-delahaye left a comment (ESAPI/esapi-java-legacy#888)
Hi @kwwall
Apologies if this is not the right place to comment on this change.
I did read the javadoc, the security bulletin, and the PoC and I am trying to understand the risk of using OracleCodec (possibly forking it should it be removed from easpi in a future release).
I'm no Java/Oracle expert so I'm sure I'm missing something. And I could not get the JavaSecLab to compile or to work online to find out where I'm wrong... But below is what confuses me:
First of all,
1\' and if(1=1,sleep(5),1)--,
when passed in to encodeForSQL, gets encoded as
1\'' and if(1=1,sleep(5),1)--
So the SQL query should be
select * from sqli where id = '1\'' and if(1=1,sleep(5),1)--'
which is safe (see below)
How can it be
select * from sqli where id = '1' and if(1=1,sleep(5),1)--
as stated in the security bulletin, which indeed would be unsafe?
But beyond that, two points I would like to mention:
• \ is the default escape character for Oracle Database
As far as I understand, \ is NOT an escape character. I think the only escape character (as far as single quotes are concerned) is ... a single quote, which is correctly escaped by OracleCodec. The author of the codec was right in 2007, and IMO, they're still right today.
So \'' in Oracle literally means the \ character followed by the ' one and
where id = '1\'' and if(1=1,sleep(5),1)--'
will match records with id (literally) 1\' and if(1=1,sleep(5),1)--
• the escape character (...) can be defined dynamically by the developer
I'm guessing you're referring to the escape clause? If so, imo, this is only valid in a like clause to override the default wildcards (% or _). Nothing to do with escaping single quotes.
Would you by any chance have some time to give a bit more details?
Many thanks in advance.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Contributor
|
I addition to Matt's reply (#888 (comment)), I just responded (#889 (reply in thread)) to your query in ESAPI GitHub Discussion #889 (which probably would have initially been a better initial spot for commenting on this.)
I suggest if you need more details, you contact the author of https://github.com/uglory-gll/javasec/ESAPI.md as it could be that you using a different JDBC driver than the security researcher did in his tests.
Other than that, I don't really know what to add that Matt hasn't already said. Lastly, if you wish to comment in the broader ESAPI context on this specific issue, please move those discussions to Discussion #889. Thanks.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.